Implementation of a look-up table for small topologies and changes in the dimensions of the groups of rare topology. - #1171
Conversation
e507af9 to
86907a2
Compare
There was a problem hiding this comment.
this function is not really returning anything (it is "filling" something maybe). mBitmap seems to be a std::array like object so I guess one could just say
std::array<char, N> getPattern() const { return mBitmap; }
There was a problem hiding this comment.
so what was the motivation for this change? (see above)
There was a problem hiding this comment.
I reverted to the array<unsigned char, N> version.
The change should have been only temporary, since I had some problems during the implementation of the last modifications to the code, but then I forgot to switch back. Now everything works fine.
There was a problem hiding this comment.
please put '{}' even around single line if-bodies (see coding rules).
There was a problem hiding this comment.
I put them. Thank you
There was a problem hiding this comment.
In the previous version I've changed your unordered_map to map just because otherwise it was failing the MacOS test. With the reintroduction of the unordered_map, failure is now back. I am sure this is a root bug, will it work for the moment with map, until we sort out the unordered_map problem?
There was a problem hiding this comment.
You don't need actually to spend time on nBytes calculation, can check directly if (nBits<9) { // small topology
There was a problem hiding this comment.
In the previous version I've changed your std::unordered_map to std::map because otherwise it was failing the PR test on MacOS. With your reintroduction of the unordered_map the failure is back. Will it work with std::map for the moment, until we sort out the problem with unordered_map (apparently a root bug)?
There was a problem hiding this comment.
Thank you @shahor02, you just anticipated me. I passed to unordered_map since I thought that the problem was solved in the meanwhile. Then, when this probIem showed up, I suspected that the reason of the problem was unordered_map. However, the strange thing is that everything compiles on my laptop, i.e. on macOS.
For the time being I will pass to map
|
Please use |
There was a problem hiding this comment.
You add +2 to a constant in multiple places. I think it would be more readable to define another constant which has the meaning of kMaxPatterBytes + 2.
9cbf0de to
b6e86be
Compare
|
Thanks. Looks good to me. @iouribelikov, @bovulpes, @shahor02 : Should we go ahead? |
|
@dberzano : The macos builds seem not to be active for the last hours. Is the machine down? |
ClusterPattern includes a bitmap with the number of rows and the number of columns in the first two bytes. The remaining bytes contain the very bitmap of the cluster. The dictionary now contains a lookup table with the group ID corresponding to the topologis whose bitmap can be contained in a single byte.
5ed6c08 to
1cdabf4
Compare
The maximum number of rows and columns for groups of rare topology is set to the highest number that can contain the complete pixel bitmap (Cluster::kMaxPatternBits).
|
@sawenzel for the record, since the question popped up, |
|
@iouribelikov , @shahor02 : Should we go ahead? |
|
@sawenzel Yes, for me it is fine |
|
@sawenzel OK. Let's merge it. |
|
Hi, it came something to my mind which I want still to check, could you please wait? Sorry...
On Jun 13, 2018 07:03, iouribelikov <notifications@github.com> wrote:
@sawenzel<https://github.com/sawenzel> OK. Let's merge it.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#1171 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AHCkDTWA5pZgWTV5toBfi9Ax_GzvtsrJks5t8J0DgaJpZM4UcieR>.
|
|
Hi, fortunately, my worries were baseless, please go ahead with merging. |
5 participants
A look-up table containing all the IDs of topologies whose bitmap is contained in 1B has been added. Thus it is possible to avoid the computation of the hashcode for the smallest (and very common) topologies when ecountered. Indeed, the LUT element is directly accessed through its 1-Byte bitmap.
Then the dimensions of the groups of rare topologies have changed. Previously, the maximum size of the pattern was 32x32 (defined in
TopologyDictionary). However, all the topologies with one dimensions exceeding this limit belonged to the group with 32 as number of row or columns. Now, the maximum number of rows/columns has been increased to the highest possible number for a topology with 512 pixels (Cluster::kMaxPatternBits).